Skip to content

[src && example] Fix client lifecycle, buffer handling, and demo init#17

Open
Sigma711 wants to merge 9 commits intoInfraSail:mainfrom
Sigma711:dev
Open

[src && example] Fix client lifecycle, buffer handling, and demo init#17
Sigma711 wants to merge 9 commits intoInfraSail:mainfrom
Sigma711:dev

Conversation

@Sigma711
Copy link
Copy Markdown
Member

@Sigma711 Sigma711 commented Jan 7, 2026

[src && example]: Apply Clang-Format and Clang-Tidy, Fix Protobuf Build, and General Code Improvements

Purpose

This PR aims to improve code quality and maintainability by applying consistent code formatting using clang-format and enforcing coding standards through clang-tidy. It also resolves a critical build issue related to generated Protobuf files and includes other general code improvements.

Changes

  • Code Formatting: Applied clang-format to all C++ source files (.h, .hpp, .c, .cc, .cpp) in src, test, and example directories, ensuring adherence to the project's style guide (Google style).
  • Static Analysis: Enabled and integrated clang-tidy into the CMake build process (TAOTU_ENABLE_CLANG_TIDY=ON) to perform static code analysis and enforce code quality rules during compilation.
  • Protobuf Build Fix: Identified and resolved an issue where clang-format was inadvertently modifying generated Protobuf files (rpc.pb.cc, rpc.pb.h), leading to compilation errors. The fix involves ensuring these generated files are not subject to clang-format's modifications by regenerating them cleanly.
  • Build System Enhancements: Updated CMake configuration to properly handle generated Protobuf files and ensure clang-tidy runs effectively on the codebase.
  • General Code Improvements: Included various minor code adjustments and fixes across example/chat_room/chat_server.cc, example/pingpong/pingpong_client.cc, src/connecting.cc, src/connecting.h, and src/rpc_codec.cc resulting from the application of clang-format.

Review Instructions

  • Verify that the code style is consistent across the codebase by checking a few files that were modified.
  • Ensure that the project builds successfully in both Debug and Release modes after pulling this PR.
    • mkdir build && cd build && cmake .. -DCMAKE_BUILD_TYPE=Debug -DTAOTU_ENABLE_CLANG_TIDY=ON && cmake --build . -j
    • mkdir build_release && cd build_release && cmake .. -DCMAKE_BUILD_TYPE=Release && cmake --build . -j
  • Confirm that there are no new clang-tidy warnings or errors introduced by these changes during the build process.
  • Specifically check the src/rpc.pb.h and src/rpc.pb.cc files after a clean build to ensure they are correctly generated and not modified by clang-format.

@Sigma711 Sigma711 requested a review from Copilot January 7, 2026 12:38
@Sigma711 Sigma711 self-assigned this Jan 7, 2026
@Sigma711 Sigma711 added the enhancement New feature or request label Jan 7, 2026

This comment was marked as outdated.

chatgpt-codex-connector[bot]

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 37 out of 37 changed files in this pull request and generated 8 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 167 to 173
Poller::~Poller() {
// Clean up user-space ops still in the queue to avoid leaks on early exit.
LOG_DEBUG("Destroying Poller, pending ops: %zu", ops_.size());
{
std::lock_guard<std::mutex> lock(ops_mutex_);
for (auto& item : ops_) {
CleanupOpContext(item.second.get());
}
ops_.clear(); // unique_ptr releases automatically.
for (auto* op : op_pool_) {
delete op;
}
op_pool_.clear();
UnregisterBuffers();
::io_uring_queue_exit(&ring_);
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Poller no longer tracks in-flight IoUringOps. In the destructor only op_pool_ is deleted, so any ops that are still in-flight at shutdown (and their context data) will be leaked, and context_deleter will never run. Consider keeping a container of in-flight ops (or a refcount) so the destructor can cancel/drain and then cleanup all remaining ops/contexts before exiting the ring.

Copilot uses AI. Check for mistakes.
::io_uring_prep_provide_buffers(
sqe, buffers_.get() + (static_cast<size_t>(bid) * kBufSize), kBufSize, 1,
kBufferGroupId, bid);
::io_uring_sqe_set_data64(sqe, 0);
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the non-buf_ring path, ReturnBuffer() prepares an IORING_OP_PROVIDE_BUFFERS SQE but never submits it. With SubmitPending(false) batching (default threshold 16), a small number of returned buffers may never get submitted promptly, causing the provided-buffer pool to remain depleted and triggering ENOBUFS/stalls. Consider forcing a submit after preparing a provide-buffers SQE (or tracking that buffer-return SQEs were queued and calling SubmitPending(true) at the end of Poll()).

Suggested change
::io_uring_sqe_set_data64(sqe, 0);
::io_uring_sqe_set_data64(sqe, 0);
// Ensure the PROVIDE_BUFFERS SQE is submitted promptly so the buffer
// actually returns to the kernel even under low activity.
SubmitPending(true);

Copilot uses AI. Check for mistakes.
Comment on lines +335 to +336
connection_ptr.release();
continue;
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

detach_connection path releases the unique_ptr without transferring ownership or deleting it, which permanently leaks the Connecting instance (and any resources it owns). If the intent is to allow shutdown without waiting for pending IO, consider explicitly handing ownership to DestroyConnectionCallback_ (if set), or adding a dedicated “deferred destroy” list that is drained until pending IO completes, instead of release().

Suggested change
connection_ptr.release();
continue;

Copilot uses AI. Check for mistakes.
Comment on lines +73 to +78
- `TAOTU_IORING_BORROWED_BUFFER_LIMIT`
- Default: `kBufCount/2` (currently `128`)
- Clamp range: `[0, kBufCount]`
- Effect: max number of recv-multishot provided buffers that can be leased
(held past the read CQE) for the borrowed-send fast path; `0` disables
leasing (borrowed send falls back to copy).
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The README documents TAOTU_IORING_BORROWED_BUFFER_LIMIT, but there is no corresponding implementation in the codebase (no reads of this env var). Also the documented default “currently 128” no longer matches kBufCount (now 1024 in Poller). Please either implement this knob or remove/adjust the documentation to match actual behavior/defaults.

Suggested change
- `TAOTU_IORING_BORROWED_BUFFER_LIMIT`
- Default: `kBufCount/2` (currently `128`)
- Clamp range: `[0, kBufCount]`
- Effect: max number of recv-multishot provided buffers that can be leased
(held past the read CQE) for the borrowed-send fast path; `0` disables
leasing (borrowed send falls back to copy).

Copilot uses AI. Check for mistakes.
Comment on lines +71 to +76
- `TAOTU_IORING_BORROWED_BUFFER_LIMIT`
- 默认值:`kBufCount/2`(当前为 `128`)
- 限制范围:`[0, kBufCount]`
- 作用:recv-multishot 的 provided-buffer 最多允许被“借用/持有”的数量上限
(读取 CQE 回调结束后暂不归还,用于借用直发);设为 `0` 可禁用借用
(借用发送会自动回退为拷贝发送)。
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

文档中提到 TAOTU_IORING_BORROWED_BUFFER_LIMIT,但代码中没有读取/实现该环境变量。另外文档写的默认值“当前为 128”也与 PollerkBufCount(现在是 1024)不一致。建议:要么补齐该配置项的实现,要么删除/修正文档以匹配实际行为与默认值。

Suggested change
- `TAOTU_IORING_BORROWED_BUFFER_LIMIT`
- 默认值:`kBufCount/2`(当前为 `128`
- 限制范围:`[0, kBufCount]`
- 作用:recv-multishot 的 provided-buffer 最多允许被“借用/持有”的数量上限
(读取 CQE 回调结束后暂不归还,用于借用直发);设为 `0` 可禁用借用
(借用发送会自动回退为拷贝发送)。

Copilot uses AI. Check for mistakes.
OPTION(TAOTU_ENABLE_CLANG_TIDY "Enable clang-tidy checks." OFF)

SET(CMAKE_CXX_FLAGS_RELEASE "-O3 -DNDEBUG")
SET(CMAKE_CXX_FLAGS_RELEASE "-O3 -DNDEBUG -march=native -flto")
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-march=native and -flto in the default Release flags can make produced binaries non-portable across CPUs and may break some toolchains/cross-compiles. Consider gating these behind an explicit CMake option (e.g. TAOTU_ENABLE_LTO / TAOTU_ENABLE_NATIVE_ARCH) rather than enabling unconditionally for all Release builds.

Copilot uses AI. Check for mistakes.
Comment on lines +337 to +338
char* msg = ::strerror_r(err, errbuf, sizeof(errbuf));
(void)msg;
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same portability issue here: assigning ::strerror_r() to char* is only valid for the GNU variant. On POSIX strerror_r returns int, so this will not compile unless _GNU_SOURCE is enabled. Prefer the existing StrError wrapper pattern or avoid assigning the return value.

Suggested change
char* msg = ::strerror_r(err, errbuf, sizeof(errbuf));
(void)msg;
(void)::strerror_r(err, errbuf, sizeof(errbuf));

Copilot uses AI. Check for mistakes.
Comment on lines +277 to +278
char* msg = ::strerror_r(err, errbuf, sizeof(errbuf));
(void)msg;
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strerror_r has two incompatible signatures (GNU returns char*, POSIX returns int). Storing its return value in a char* here will fail to compile when _GNU_SOURCE is not enabled. Consider using the existing StrError wrapper pattern used in src/connector.cc/src/connecting.cc, or call strerror_r without assigning its return value and rely on errbuf.

Suggested change
char* msg = ::strerror_r(err, errbuf, sizeof(errbuf));
(void)msg;
(void)::strerror_r(err, errbuf, sizeof(errbuf));

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants